Skip to content

Move checking placeholder types in return types to typeck - #153243

Open
Zoxc wants to merge 7 commits into
rust-lang:mainfrom
Zoxc:fn-sig-placeholder-move
Open

Move checking placeholder types in return types to typeck#153243
Zoxc wants to merge 7 commits into
rust-lang:mainfrom
Zoxc:fn-sig-placeholder-move

Conversation

@Zoxc

@Zoxc Zoxc commented Mar 1, 2026

Copy link
Copy Markdown
Contributor

View all comments

This moves checking placeholder types in return types from the fn_sig query to typeck. typeck computes the return value of the body which is used for error suggestions. This is done to prevent a query cycle between fn_sig and typeck.

Currently fn_sig is marked with cycle_delay_bug to deal with this cycle, but I'm not sure if it's sufficient as we need the query we resume to have a Value impl, which may not be fn_sig.

Functions such as fn foo() -> _ will now return as fn foo() -> <error> from fn_sig instead of using the return type of their bodies. This can hide some later errors, but that seems like a minor downside.

This is also a step towards removing query cycle recovery.

@rustbot

rustbot commented Mar 1, 2026

Copy link
Copy Markdown
Collaborator

HIR ty lowering was modified

cc @fmease

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 1, 2026
@rustbot

This comment was marked as outdated.

Comment thread compiler/rustc_hir_analysis/src/collect.rs Outdated
Comment thread compiler/rustc_hir_analysis/src/collect.rs Outdated
@dingxiangfei2009

dingxiangfei2009 commented Mar 1, 2026

Copy link
Copy Markdown
Contributor

Thank you for contribution! 🙇

@bors r+ rollup=always

@rust-bors

rust-bors Bot commented Mar 1, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 8ddc609 has been approved by dingxiangfei2009

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2026
@fmease

fmease commented Mar 1, 2026

Copy link
Copy Markdown
Member

Currently fn_sig is marked with cycle_delay_bug to deal with this cycle, but I'm not sure if it's sufficient as we need the query we resume to have a Value impl, which may not be fn_sig.

I'm struggling to understand why you didn't remove cycle_delay_bug from query fn_sig then (plus the Value impl for PolyFnSig)? Is some other place still relying on fn_sig delaying cycle errors as bugs?

tcx: TyCtxt<'tcx>,
item_def_id: LocalDefId,
tainted_by_errors: Cell<Option<ErrorGuaranteed>>,
suppress_placeholder_errors: Cell<bool>,

@fmease fmease Mar 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be frank, I'm not super stoked about adding a hyper-specific field to ItemCtxt plus an "anonymous" Boolean parameter to various functions. This solution feels blunt(?) and slightly hacky.

I haven't had the time to think about alternative solutions yet if there are any. I believe it should be possible to localize this code a lot more even if that meant ~reverting parts of PR #125819 (before which this recovery was 'more local' IIRC).

(personally speaking I'm generally unhappy about this super invasive & complex recovery logic for an 'incredibly niche' user error; it's already caused me headaches in the past)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could possibly split out placeholder errors into a separate HIR visitor, then just skip visiting return types as appropriate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(personally speaking I'm generally unhappy about this super invasive & complex recovery logic for an 'incredibly niche' user error; it's already caused me headaches in the past)

I think we occasionally go too far in our quest for high quality error messages. If an obscure error message is causing problems for perf or code complexity I think removing it is totally reasonable.

Just to clarify: which user error is the incredibly niche one?

@fmease fmease Mar 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify: which user error is the incredibly niche one?

So I don't know how often beginner or average users encounter this error or how helpful they find the suggestion, I'm talking about code like const C = 1; (no type annotation), static S: _ = 1; (_ in item signatures), fn f() -> _ { 0 } but also static A: [&str; _] = [];.

I get that they can be quite useful, so maybe I've exaggerated. Still, personally speaking I just never use _ like this to obtain the right type from the compiler, so I'm a bit biased.

It's just that I know the implementation is quite unwieldy and hairy. Most crucially calling typeck instead of type_of on items that usually mandate a type signature is prone to query cycles (e.g, I most recently had to fight them in RUST-143029) which can be quite hindering when developing new features.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could possibly split out placeholder errors into a separate HIR visitor, then just skip visiting return types as appropriate.

ItemCtxt had a few more users then I expected, so this might not be a good idea as it could be easy to skip the separate visitor.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be frank, I'm not super stoked about adding a hyper-specific field to ItemCtxt

agreed on this, even though it seems like the latter half seems to be "fixed". What does this PR look like without this?

@fmease fmease Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in reply to #153243 (comment)

Without this flag we emit generic error diagnostics per _ in return types plus one diagnostic that actually contains the specialized suggestion with multiple primary highlights.

Using diagnostic stashing + downgrading to delayed bugs I was able go get rid of this flag while preserving the desired behavior. I'll push that later to this PR.

@fmease

fmease commented Mar 1, 2026

Copy link
Copy Markdown
Member

Sorry, I'm gonna unapprove this for now due to #153243 (comment) (question) and #153243 (comment) (semi-actionable concern)

@bors r-

@rust-bors rust-bors Bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 1, 2026
@fmease fmease self-assigned this Mar 1, 2026
@Zoxc

Zoxc commented Mar 2, 2026

Copy link
Copy Markdown
Contributor Author

I'm struggling to understand why you didn't remove cycle_delay_bug from query fn_sig then (plus the Value impl for PolyFnSig)? Is some other place still relying on fn_sig delaying cycle errors as bugs?

It unhides unrelated cycle errors, so I kept it out here to keep the PR smaller.

@rust-bors

This comment has been minimized.

@Zoxc
Zoxc force-pushed the fn-sig-placeholder-move branch from 8ddc609 to 3424ede Compare March 16, 2026 14:10
@rustbot

This comment has been minimized.

@Zoxc

Zoxc commented Mar 27, 2026

Copy link
Copy Markdown
Contributor Author

@fmease Can this get a re-evaluation with the extra parameter now removed?

@fmease fmease added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 28, 2026
@rust-bors

This comment has been minimized.

@rust-bors

This comment has been minimized.

@teor2345 teor2345 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes on changes in PR #155223.

View changes since this review

Comment thread compiler/rustc_hir_analysis/src/collect.rs
@rustbot

rustbot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rust-log-analyzer

This comment has been minimized.

Comment thread compiler/rustc_hir_analysis/src/collect.rs Outdated
);
} else if let Some(sugg) = rustc_hir_analysis::suggest_impl_trait(
&tcx.infer_ctxt().build(ty::TypingMode::non_body_analysis()),
tcx.param_env(def_id),

@fmease fmease Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to myself: We could avoid the query call by accepting the param_env from the caller. Furthermore, we might be able to reuse the InferCtxt from the caller's FnCtxt, although that one is "typeck_for_body", not "non_body_analysis" & maybe the InferCtxt is bricked after writeback?

View changes since the review

tcx: TyCtxt<'tcx>,
item_def_id: LocalDefId,
tainted_by_errors: Cell<Option<ErrorGuaranteed>>,
suppress_placeholder_errors: Cell<bool>,

@fmease fmease Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in reply to #153243 (comment)

Without this flag we emit generic error diagnostics per _ in return types plus one diagnostic that actually contains the specialized suggestion with multiple primary highlights.

Using diagnostic stashing + downgrading to delayed bugs I was able go get rid of this flag while preserving the desired behavior. I'll push that later to this PR.

@fmease
fmease force-pushed the fn-sig-placeholder-move branch 2 times, most recently from f883e4e to 4f8d202 Compare September 10, 2026 22:37
Instead of manually keeping track of whether to suppress "bad placeholder"
diagnostics via flag `ItemCtxt.suppress_placeholder_errors` just stash
the relevant diagnostics beforehand & steal them later on to delay them
as bugs.
@fmease

fmease commented Sep 10, 2026

Copy link
Copy Markdown
Member

Since I've now contributed substantial changes to this PR in form of 3 commits, I'd like another reviewer to take a look at all the changes.

r? @nnethercote maybe? feel free to reassign

@rustbot rustbot assigned nnethercote and unassigned fmease Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants